Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support certificate role mappings #37269

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Support certificate role mappings #37269

merged 1 commit into from
Dec 5, 2023

Conversation

sberyozkin
Copy link
Member

Fixes #23683.

Tests to follow, also I thought I could avoid registering an augmentor and just pass the roles directly to the default X509 identity provider.

If these mappings are not comping from the file/classpath resource then a custom SecurityIdentityAugmentor will help.

I'll open for review later but early feedback is welcome

@sberyozkin sberyozkin mentioned this pull request Nov 22, 2023
Closed
@sberyozkin sberyozkin force-pushed the certificate-role-mapping-support branch 2 times, most recently from 350ff1b to 0f7deec Compare November 24, 2023 20:15
@sberyozkin sberyozkin marked this pull request as ready for review November 24, 2023 20:16
@sberyozkin
Copy link
Member Author

sberyozkin commented Nov 24, 2023 •
edited
Loading

I wanted to modify integration-tests/vertx-http but it is the only test which tests a truststore alias option which I thought was important to retain while for this PR tests the whole truststore should be trusted to test the client certificates with different mappings

@quarkus-bot

This comment has been minimized.

Sign in to view
michalvavrik
michalvavrik reviewed Nov 25, 2023
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, few notes, two important, others you can ignore

...untime/src/main/java/io/quarkus/vertx/http/runtime/security/MtlsAuthenticationMechanism.java Outdated Show resolved Hide resolved
...-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityRecorder.java Outdated Show resolved Hide resolved
...-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityRecorder.java Outdated Show resolved Hide resolved
integration-tests/mtls-certificates/pom.xml Outdated Show resolved Hide resolved
integration-tests/mtls-certificates/pom.xml Outdated Show resolved Hide resolved
...on-tests/mtls-certificates/src/test/java/io/quarkus/it/vertx/CertificateRoleMappingTest.java Show resolved Hide resolved
@sberyozkin
Copy link
Member Author

Thanks @michalvavrik, let me deal with the comments, all of them make sense

@sberyozkin sberyozkin force-pushed the certificate-role-mapping-support branch from 0f7deec to c91f800 Compare November 25, 2023 17:39
@quarkus-bot

This comment has been minimized.

Sign in to view
@Ladicek
Copy link
Contributor

Ladicek commented Nov 28, 2023

I'm sure someone will come in the future asking to use a different certificate attribute :-), but CN seems good enough for a start.

@quarkus-bot

This comment has been minimized.

Sign in to view
dmlloyd
dmlloyd reviewed Dec 4, 2023
View reviewed changes
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, for what that's worth, except for one problem around loading the properties file.

(In case anyone is wondering: I reviewed this code mainly because I was curious having tinkered with this kind of thing a bit back in my days contributing to the WildFly Elytron project.)

...-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityRecorder.java Outdated Show resolved Hide resolved
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/AuthRuntimeConfig.java Outdated Show resolved Hide resolved
...-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityRecorder.java Outdated Show resolved Hide resolved
@sberyozkin
Copy link
Member Author

Thanks @dmlloyd, your feedback is very welcome

@sberyozkin sberyozkin force-pushed the certificate-role-mapping-support branch from f546eab to 217511a Compare December 5, 2023 13:11
@sberyozkin
Copy link
Member Author

@dmlloyd Hi David, I think I've addressed your comments, which was easy since your typed how to address them :-), if there is anything else that you'd like to change, let me know please

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 5, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member Author

Thanks @dmlloyd @michalvavrik and @Ladicek for the reviews. Let me merge and as always, things can be improved/optimized going forward

@sberyozkin sberyozkin merged commit 2aeab7f into quarkusio:main Dec 5, 2023
49 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Dec 5, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 5, 2023
@sberyozkin sberyozkin deleted the certificate-role-mapping-support branch December 28, 2023 19:59
@michalvavrik michalvavrik mentioned this pull request Jan 3, 2024
Merged
@cescoffier
Copy link
Member

@sberyozkin Just made aware of this. Why didn't you reuse the mechanism defined in https://datatracker.ietf.org/doc/html/rfc5755#section-4.4.5 instead of using a custom attribute (which lead to interoperability issue)

@cescoffier
Copy link
Member

At least it should be X-Roles (or a valid syntax).

@sberyozkin
Copy link
Member Author

@cescoffier Sorry, I have missed it.

Why didn't you reuse the mechanism defined in https://datatracker.ietf.org/doc/html/rfc5755#section-4.4.5 instead of using a custom attribute (which lead to interoperability issue)

We don't expect here CName to contain any roles, the requirement this PR addresses is only about using Cname as a key to pick up the deployment specific roles from the local file.

As far as expecting the certificate already have the roles, in the X-Roles attribute, please open an enhancement request if you'd like, interesting idea, it won't be in conflict with this PR

@cescoffier
Copy link
Member

Shouldn't it use SAN and not the CN? Like for SNI.

@sberyozkin
Copy link
Member Author

@cescoffier This PR addresses a specific requirement, use CName as a map key.

But sure, we can make the attribute name configurable, sounds good to me.

@sberyozkin sberyozkin mentioned this pull request Mar 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladicek Ladicek Ladicek left review comments

@dmlloyd dmlloyd dmlloyd approved these changes

@michalvavrik michalvavrik michalvavrik approved these changes

@stuartwdouglas stuartwdouglas Awaiting requested review from stuartwdouglas

Assignees
No one assigned
Labels
area/security area/vertx kind/enhancement New feature or request release/noteworthy-feature triage/backport-3.2
Projects
None yet
Milestone
3.7.0.CR1
Development

Successfully merging this pull request may close these issues.

Support role mapping for Client certificates
5 participants
@sberyozkin @Ladicek @cescoffier @dmlloyd @michalvavrik